[CELEBORN-2257] Fixed remote disks not being reported on registration#1
[CELEBORN-2257] Fixed remote disks not being reported on registration#1Dzeri96 wants to merge 2 commits intoQbeast-io:mainfrom
Conversation
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
I would add some unit test that shows the problem and that fails without this patch.
From the code it is not clear to me that the change actually solves the issue.
I am not even sure if the Worker needs to advertise the virtual dummy disks to the Master, is this useful ?
| } | ||
| } | ||
|
|
||
| // TODO: Move all accesses to Type.getMask() |
There was a problem hiding this comment.
please do not add new TODOs
or at least create an issue and link to it
in this case I think that it is better to not add the TODO
| for (newDisk <- newDiskInfos.values().asScala) { | ||
| val mountPoint: String = newDisk.mountPoint | ||
| val curDisk = diskInfos.get(mountPoint) | ||
| // TODO: Avoid function side-effect |
There was a problem hiding this comment.
same comment as above: TODOs are not a good practice for a project handled by a community
master/src/main/java/org/apache/celeborn/service/deploy/master/SlotsAllocator.java
Outdated
Show resolved
Hide resolved
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala
Show resolved
Hide resolved
|
@eolivelli So this is important, because in Regarding tests, I wanted to submit the PR and ask for their opinion on what kind of test to write. My code fixes a problem that's present when a Worker and a Master communicate, so maybe an integration tests makes more sense. |
The best test is anything that reproduces the problem, so that you can demonstrate that your code actually fixes it. |
|
The problem that I have is that I don't have the right test infrastructure to test this. This is why I think it's time to open a PR with the main repo and ask them there. |
Dzeri96
force-pushed
the
CELEBORN-2257
branch
from
f3c8c90 to
d5ae63a
Compare
2 participants
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR resolve a correctness bug?
Yes
Does this PR introduce any user-facing change?
No
How was this patch tested?
Test suite. The code hasn't been deployed so far.